Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework of monitoring output execution together with execution phase #312

Merged

Conversation

sok82
Copy link
Contributor

@sok82 sok82 commented Sep 12, 2024

After hours of struggling with monitoring of output execution using OutputAreaModel changes I reworked previous functionality connected with monitoring to simple and convenient form of attaching one simple listener to output enabling user to monitor output changes.

Changes addressing rework of #290.

Result is demonstrated in new example called OutputWithMonitoring

It works like this - attaching simple listener which gets new output area model and execution phase in one object

...
  const handleExecutionPhaseChanged = (phaseOutput: IExecutionPhaseOutput) => {
    switch (phaseOutput.executionPhase) {
      case ExecutionPhase.running:
        const log = [new Date().toISOString() + ' EXECUTION PHASE - RUNNING'];
        setExecutionLog(log);
        break;
      case ExecutionPhase.completed:
        setExecutionLog(executionLog => [
          ...executionLog,
          new Date().toISOString() +
            ' EXECUTION PHASE - COMPLETED and output ' +
            JSON.stringify(phaseOutput.outputModel?.toJSON()),
        ]);
        break;
      case ExecutionPhase.completed_with_error:
        setExecutionLog(executionLog => [
          ...executionLog,
          new Date().toISOString() +
            ' EXECUTION PHASE - COMPLETED_WITH_ERROR and output ' +
            JSON.stringify(phaseOutput.outputModel?.toJSON()),
        ]);
        break;
    }
  };

...
<Output
        autoRun={false}
        code={code}
        id={id}
        kernel={defaultKernel}
        executeTrigger={execTrigger}
        outputs={output ? output : []}
        onExecutionPhaseChanged={handleExecutionPhaseChanged}
        suppressCodeExecutionErrors={true}
        showEditor
      />

Below you can find sceenshot with visual representation of monitored events - it works both when we have new output or when executor generates no output at all, or when execution completes with error

image

@sok82
Copy link
Contributor Author

sok82 commented Sep 12, 2024

I checked few other examples linked with code execution (for example KernelExecution) - looks like they all work as before

@sok82
Copy link
Contributor Author

sok82 commented Sep 12, 2024

Hey @echarles

Do you have time today to check this?

I hope it's final (in short term) improvement that I need for my project to go further...

@echarles
Copy link
Member

I am working atm on a branch with quite some changes. I will review and comment by tomrrow your changes, hopefully there will be no merge conflict, not sure.

@echarles
Copy link
Member

@sok82 #313 is just merged has has introduced conflict on your PR. Hopefully just change of the import. Could you rebase before I review it? Sorry again for the pain.

@sok82 sok82 force-pushed the feat-290-fix-for-monitoring-execution-phase branch from 738334b to b8a95bd Compare September 13, 2024 14:41
@sok82
Copy link
Contributor Author

sok82 commented Sep 13, 2024

@echarles I fixed conflicts, it seems like ok now.

Can you squash all commits before merging them into main after checking PR?

Copy link
Member

@echarles echarles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGMT Thx @sok82

@echarles echarles merged commit 90b6ddc into datalayer:main Sep 13, 2024
2 of 4 checks passed
@echarles
Copy link
Member

Just released 0.18.8. Thx @sok82

@sok82 sok82 deleted the feat-290-fix-for-monitoring-execution-phase branch September 13, 2024 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants